Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Consolidate and update pytest options in pyproject.toml #1773

Merged
merged 25 commits into from
Feb 15, 2022

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Feb 9, 2022

Description

Resolves #1685

Remove .coveragerc and consolidate pytest options to pyproject.toml (this results in --cov-branch getting added.) In doing so, also apply the Configuring pytest recommendations from Scikit-HEP that Henry wrote.

  • -ra includes a report after pytest runs with a summary on all tests except those that passed. From pytest --help:
    • -r chars show extra test summary info as specified by chars: (f)ailed, (E)rror, (s)kipped, (x)failed, (X)passed, (p)assed, (P)assed with output, (a)ll except passed (p/P), or (A)ll. (w)arnings are enabled by default (see --disable-warnings), 'N' can be used to reset the list. (default: 'fE').

    • This allows for removal of -r sx in all the CI jobs.
  • -Wd enables all warnings.
  • --showlocal prints locals in tracebacks.
  • --strict-markers will complain if you use an unspecified fixture.
  • --strict-config will raise an error if there is a mistake in the pytest config.
  • log_cli_level = "info" reports INFO and above log messages on a failure.
  • filterwarnings = ["error"] sets all warnings to be errors and allows for some warnings to be ignored with -W warn control syntax.
  • Remove tests/__init__.py as "there’s not often a reason to make the test directory importable, and it can confuse package discovery algorithms."

To deal with the fact that the minimum supported dependencies workflow is using intentionally older releases that will have different behaviors and warnings then the rest of the tests, use the --override-ini pytest option to override the filterwarnings option with an empty list for that workflow

pytest --override-ini filterwarnings=

though also set warning control to 'default' to see the warnings.

Also remove the now unused nbQA options for black. (Amends PR #1598)

(Thanks to @alexander-held for reminding me about branch coverage in general.)

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Remove .coveragerc and consolidate pytest options to pyproject.toml.
* Apply 'configuring pytest' recommendations for pytest from Scikit-HEP
(c.f. https://scikit-hep.org/developer/pytest#configuring-pytest ).
   - '-ra' includes a report after pytest runs with a summary on all tests
     except those that passed. From 'pytest --help':
     > -r chars: show extra test summary info as specified by chars: (f)ailed,
     > (E)rror, (s)kipped, (x)failed, (X)passed, (p)assed, (P)assed with output,
     > (a)ll except passed (p/P), or (A)ll. (w)arnings are enabled by default
     > (see --disable-warnings), 'N' can be used to reset the list. (default: 'fE').
   - '--showlocal' prints locals in tracebacks.
   - '--strict-markers' will complain if you use an unspecified fixture.
   - '--strict-config' will raise an error if there is a mistake in the pytest config.
   - 'log_cli_level = "info"' reports INFO and above log messages on a failure.
   - 'filterwarnings = ["error"]' sets all warnings to be errors and allows for
     some warnings to be ignored with -W warn control syntax.
     (c.f. https://docs.python.org/dev/using/cmdline.html#cmdoption-W )
* Remove `tests/__init__.py` as no reason to make the tests directory importable.
* Remove '-r sx' from pytest calls in CI jobs as pyproject.toml now applies '-ra'.
* Use 'with pytest.warns' to assert expected warnings in tests.
(c.f. https://docs.pytest.org/en/7.0.x/how-to/capture-warnings.html#warns )
* Override error on filterwarnings for the 'minimum supported dependencies' GHA workflow
as it is testing for the oldest releases that work with the latest API, not the oldest
releases that are warning free.
* Remove unused nbQA options for black from pyproject.toml.
   - Amends PR #1598

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #1773 (9a42ff4) into master (b091985) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1773   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files          64       64           
  Lines        4270     4270           
  Branches      683      683           
=======================================
  Hits         4190     4190           
  Misses         46       46           
  Partials       34       34           
Flag Coverage Δ
contrib 26.25% <ø> (ø)
doctest 60.58% <ø> (ø)
unittests 96.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b091985...9a42ff4. Read the comment docs.

@matthewfeickert matthewfeickert changed the title test: Add --cov-branch option to pytest options for redundancy test: Add --cov-branch option to pytest options Feb 9, 2022
https://mail.python.org/pipermail/python-dev/2010-April/099116.html

> -Wd enables all warnings.  It adds 'd' to sys.warnoptions, which in turn
> adds a new first entry to _warnings.filters which matches all warnings and
> enables the "default" behavior, which is to show it once per execution of
> the Python interpreter.

pytest options:

> reporting:
> ...
> -r chars              show extra test summary info as specified by chars:
> (f)ailed, (E)rror, (s)kipped, (x)failed, (X)passed, (p)assed, (P)assed with
> output, (a)ll except passed (p/P), or (A)ll. (w)arnings are enabled by default
> (see --disable-warnings), 'N' can be used to reset the list. (default: 'fE').
@matthewfeickert matthewfeickert marked this pull request as draft February 9, 2022 21:36
@matthewfeickert matthewfeickert force-pushed the tests/enable-branch-in-pytest-options branch from 352a702 to 2b71f9d Compare February 9, 2022 21:37
@matthewfeickert matthewfeickert changed the title test: Add --cov-branch option to pytest options test: Consolidate and update pytest options in pyproject.toml Feb 9, 2022
@matthewfeickert matthewfeickert marked this pull request as ready for review February 9, 2022 22:39
@matthewfeickert matthewfeickert added chore Other changes that don't modify src or test files CI CI systems, GitHub Actions labels Feb 9, 2022
@matthewfeickert
Copy link
Member Author

@kratsg @lukasheinrich Can you also run some pytest runs locally with this PR to verify that you're happy with how the new options render output now?

@henryiii
Copy link
Member

henryiii commented Feb 9, 2022

I'd swap "-Wd enables all warnings" for filterwarnings = ["error"] (which can also have a list of allowed warnings to ignore)

@matthewfeickert
Copy link
Member Author

I'd swap "-Wd enables all warnings" for filterwarnings = ["error"] (which can also have a list of allowed warnings to ignore)

Thanks for this recommendation @henryiii. Adding this has been quite insightful and is going to lead to a lot of Issues being opened!

@henryiii
Copy link
Member

henryiii commented Feb 10, 2022

"setuptools>=30.3.0" should be "setuptools>=42"; anything before 42 was too buggy to be used with pyproject.toml (40-42), or didn't support pyproject.toml at all. (technically, PEP 517)

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 10, 2022

@henryiii one question. For the

name: Minimum supported dependencies

workflow where we test the API against the package versions in tests/constraints.txt we don't really care if there are warnings or not, but just what is the oldest versions of our dependencies that will still run with the API.

Do you know of any way to disable filterwarnings from the command line?

pytest -r sx --ignore tests/benchmarks/ --ignore tests/contrib --ignore tests/test_notebooks.py tests/

I'm a bit worried it isn't possible as pytest --help specifies that filterwarnings is "Processed after -W/--pythonwarnings".

[pytest] ini-options in the first pytest.ini|tox.ini|setup.cfg file found:

  markers (linelist):   markers for test functions
  empty_parameter_set_mark (string):
                        default marker for empty parametersets
  norecursedirs (args): directory patterns to avoid for recursion
  testpaths (args):     directories to search for tests when no files or directories are given in the command line.
  filterwarnings (linelist):
                        Each line specifies a pattern for warnings.filterwarnings. Processed after -W/--pythonwarnings.

Is there any less aggressive solution than just ripping it out?

        sed -i '/"error",/d' pyproject.toml
        pytest --ignore tests/benchmarks/ --ignore tests/contrib --ignore tests/test_notebooks.py tests/

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 10, 2022

Is there any less aggressive solution than just ripping it out?

After reading

$ pytest --help
...
test session debugging and configuration:
  --basetemp=dir        base temporary directory for this test run.(warning: this directory is removed if it exists)
  -V, --version         display pytest version and information about plugins. When given twice, also display information about plugins.
  -h, --help            show help message and configuration info
  -p name               early-load given plugin module name or entry point (multi-allowed).
                        To avoid loading of plugins, use the `no:` prefix, e.g. `no:doctest`.
  --trace-config        trace considerations of conftest.py files.
  --debug=[DEBUG_FILE_NAME]
                        store internal tracing debug information in this log file.
                        This file is opened with 'w' and truncated as a result, care advised.
                        Defaults to 'pytestdebug.log'.
  -o OVERRIDE_INI, --override-ini=OVERRIDE_INI
                        override ini option with "option=value" style, e.g. `-o xfail_strict=True -o cache_dir=cache`.

I see that

--override-ini filterwarnings=

works.

Copy link
Member

@alexander-held alexander-held left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the motivation behind filterwarnings = ["error"], I just wonder whether it is worth the extra maintenance burden to keep the list of ignores up-to date. Maybe I am overestimating the effort though, given that it seems that (at least right now) there are not a lot of things that are only due to the dependencies.

Very minor point: is there a good way to clean up the warning ignore settings periodically or automatically (if they are not used)? Assuming that something like the true_divide:RuntimeWarning would be fixed by re-designing a test (maybe even accidentally), it would probably be good to also remove that entry. When a new test will be designed in the future, the warning could be caught again and perhaps be avoided directly by re-designing the new test.

@matthewfeickert
Copy link
Member Author

matthewfeickert commented Feb 10, 2022

Maybe I am overestimating the effort though, given that it seems that (at least right now) there are not a lot of things that are only due to the depdencencies.

@alexander-held That is correct that there are quite a few things that either would get their own issue to get fixed later, like changing all instances of

json.load(open(datadir.join("abc.json")))

to

with open(datadir.join("abc.json")) as spec_file:
     spec = json.load(spec_file)

to avoid things like

pytest.PytestUnraisableExceptionWarning:Exception ignored in: <_io.FileIO [closed]>

and some of the others might be able to be dealt with, but some might not be possible to deal with easily because of the nature of hitting walls at times.

Very minor point: is there a good way to clean up the warning ignore settings periodically or automatically (if they are not used)? Assuming that something like the true_divide:RuntimeWarning would be fixed by re-designing a test (maybe even accidentally), it would probably be good to also remove that entry. When a new test will be designed in the future, the warning could be caught again and perhaps be avoided directly by re-designing the new test.

No, which is one of the reasons why ideally as many of these ignores in pyproject.toml should get their own Issues to become with pytest.warns context managers that would fail if there isn't a warning produced, like in tests/test_optim.py

    with pytest.warns(
        OptimizeWarning, match="Unknown solver options: arbitrary_option"
    ):
        assert pyhf.infer.mle.fit(data, model).tolist()

@matthewfeickert
Copy link
Member Author

@kratsg @lukasheinrich I'm going to merge this to keep things going, but if there are any parts of this that you have questions on or want reverted we can iterate here or in an new Issue. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Other changes that don't modify src or test files CI CI systems, GitHub Actions tests pytest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: some possible cleanup / extra testing
3 participants